Skip to content

feat(eval): add measurement manifests for online scoring - #722

Merged
ascerra merged 13 commits into
mainfrom
feat/eval-measurements-manifests
Aug 20, 2026
Merged

feat(eval): add measurement manifests for online scoring#722
ascerra merged 13 commits into
mainfrom
feat/eval-measurements-manifests

Conversation

@ascerra

@ascerra ascerra commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Add default measurement manifests for stock fullsend agents under eval/measurements/. These are the online-scoring policy that ships with the agents (“here is code, and here is what we measure on wild code runs”).

Companion engine PR: fullsend-ai/fullsend#6036 (ADR 0087, fullsend eval-measure, EM-001 trace_fitness).

Ownership (please read)

Concern Repo
Scorer implementations (Go), parser, CLI, job wiring fullsend#6036
Default manifests (this PR) agents eval/measurements/<agent>.yaml
Org overrides / BYOA Consumer FULLSEND_DIR
  • Managed jobs resolve local ${FULLSEND_DIR}/eval/measurements/${AGENT}.yaml if present, else fetch agents@v0 from this path. Stock-agent users do not copy these files into every repo.
  • Local files are for override, opt-out, or custom agents only.
  • This PR is policy only (YAML). Executable scoring stays in fullsend because the measure CLI reads run-telemetry.jsonl produced by fullsend.
  • Change guide: new Go scorer / (future) new declarative assert: → fullsend; new id / enable / disable / thresholds for a stock agent on an existing scorer → this repo; org-specific policy → consumer override.

Changes

  • eval/measurements/{code,fix,prioritize,retro,review,triage}.yaml — each enables trace_fitness (em-001) @ version 1
  • eval/measurements/README.md — ownership, resolution, when to PR where
  • eval/README.md — short section + links to fullsend#6036 / ADR 0087
  • Scribe intentionally omitted (no forge work-item identity for EM-001 today)

Test plan

  • Manifest YAML shape matches fullsend registry loader (agent + measurements[].{id,scorer,version})
  • Scribe excluded on purpose
  • Cross-check with fullsend#6036 / ADR 0087 before merge
  • After merge, ensure v0 (or the pin fullsend uses) includes these paths so wild runs stop skipping measure

Notes for reviewers

  • Functional eval fixtures under eval/<agent>/ are unchanged and separate.
  • Until this lands on the ref fullsend fetches (v0), measure steps skip fail-open — that is expected.

@ascerra
ascerra requested a review from a team as a code owner August 10, 2026 11:45
@qodo-code-review

qodo-code-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Add per-agent measurement manifests for online scoring (trace_fitness em-001)

✨ Enhancement 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add default per-agent measurement manifests for managed-job online scoring.
• Enable trace_fitness (em-001 v1) for six stock agents; omit others (e.g., scribe).
• Document manifest ownership, resolution order, and separation from functional eval harness.
Diagram

graph TD
  J(["Managed agent job"]) --> CLI["fullsend eval-measure"]
  CLI --> D{"Local manifest?"}
  D -->|"yes"| L["FULLSEND_DIR manifest"]
  D -->|"no"| R["agents@v0 manifest"]
  CLI --> T[("run-telemetry.jsonl")] --> S["trace_fitness scorer\n(em-001 v1)"] --> O[("Measurement results")]

  subgraph Legend
    direction LR
    _proc["Process/CLI"] ~~~ _dec{"Decision"} ~~~ _data[("Data")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Bake defaults into fullsend (no agents@v0 fetch)
  • ➕ Single release artifact controls both scorer code and default policy
  • ➕ No reliance on agents@v0 pin being updated to enable scoring
  • ➖ Harder for agents repo to own/iterate on policy independently
  • ➖ Forces fullsend release for simple enable/disable/threshold policy changes
2. Single manifest with per-agent sections
  • ➕ Fewer files to manage/version together
  • ➕ Easier to diff global policy changes across agents
  • ➖ Makes per-agent overrides less discoverable and more conflict-prone
  • ➖ Resolution logic becomes more complex (merge/selection vs direct fetch)
3. Remote registry service for measurement policy
  • ➕ Policy updates without Git pins; central governance and rollout controls
  • ➕ Can support targeting/experiments by org/repo
  • ➖ Adds new infrastructure dependency and failure modes
  • ➖ Overkill for initial ship; higher operational complexity

Recommendation: Keep the current approach: per-agent YAML manifests in this repo fetched via agents@v0 with explicit FULLSEND_DIR override. It cleanly separates engine/scorer implementation (fullsend) from policy defaults (agents), keeps overrides simple, and matches the ownership model described in the docs.

Files changed (8) +106 / -0

Documentation (2) +70 / -0
README.mdDocument measurement manifests as online-scoring policy +13/-0

Document measurement manifests as online-scoring policy

• Adds a new section explaining that 'eval/measurements/' defines default online-scoring policy for managed jobs. Clarifies separation from functional eval harness scenarios and links to the companion fullsend PR and ADR.

eval/README.md

README.mdAdd measurements README covering ownership and resolution +57/-0

Add measurements README covering ownership and resolution

• Introduces documentation for what measurement manifests are, why they live with agents, and how managed jobs resolve local overrides vs agents@v0 defaults. Defines responsibility boundaries between fullsend (scorer code) and agents (policy) and lists initial agent coverage for em-001.

eval/measurements/README.md

Other (6) +36 / -0
code.yamlAdd default manifest for code agent (em-001 v1) +6/-0

Add default manifest for code agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'code' agent.

eval/measurements/code.yaml

fix.yamlAdd default manifest for fix agent (em-001 v1) +6/-0

Add default manifest for fix agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'fix' agent.

eval/measurements/fix.yaml

prioritize.yamlAdd default manifest for prioritize agent (em-001 v1) +6/-0

Add default manifest for prioritize agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'prioritize' agent.

eval/measurements/prioritize.yaml

retro.yamlAdd default manifest for retro agent (em-001 v1) +6/-0

Add default manifest for retro agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'retro' agent.

eval/measurements/retro.yaml

review.yamlAdd default manifest for review agent (em-001 v1) +6/-0

Add default manifest for review agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'review' agent.

eval/measurements/review.yaml

triage.yamlAdd default manifest for triage agent (em-001 v1) +6/-0

Add default manifest for triage agent (em-001 v1)

• Adds a per-agent measurement manifest selecting 'trace_fitness' (id em-001) version 1 for the 'triage' agent.

eval/measurements/triage.yaml

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 11:46 AM UTC · Ended 11:51 AM UTC

Commit: de3a60a · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (1)

Grey Divider


Remediation recommended

1. ID casing inconsistency ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The measurement manifests set the trace_fitness measurement id to em-001, but the measurement
README refers to the same measurement as EM-001, creating ambiguity about the canonical identifier
to use in configs/docs/tooling.
Code

eval/measurements/code.yaml[R3-5]

+  - id: em-001
+    scorer: trace_fitness
+    version: 1
Relevance

●●● Strong

Trivial consistency fix; team has accepted casing-alignment changes in nearby review history.

PR-#592

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The repo’s new measurement README explicitly calls the scorer “EM-001” while each new manifest
defines the measurement id as “em-001”, so the PR introduces two different canonical spellings for
the same measurement identifier.

eval/measurements/README.md[10-11]
eval/measurements/code.yaml[1-5]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The measurement ID is referenced with different casing in different newly-added sources: manifests use `em-001`, while documentation calls it `EM-001`. This makes it unclear which identifier is canonical.

## Issue Context
All per-agent manifests added in this PR use `id: em-001`, but the README in the same directory describes the scorer as `EM-001`.

## Fix Focus Areas
- eval/measurements/README.md[10-11]
- eval/measurements/code.yaml[1-5]
- eval/measurements/fix.yaml[1-5]
- eval/measurements/prioritize.yaml[1-5]
- eval/measurements/retro.yaml[1-5]
- eval/measurements/review.yaml[1-5]
- eval/measurements/triage.yaml[1-5]

## Proposed fix
Pick one canonical representation (either `EM-001` everywhere or `em-001` everywhere) and update both:
1) the documentation string in `eval/measurements/README.md`, and
2) all manifest `id:` fields (if needed) to match.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Missing linked issue authorization 📜 Skill insight § Compliance
Description
This PR introduces a new measurement-manifest feature and adds 20+ lines of new configuration/docs,
but the provided PR description does not link an authorizing issue. This violates the requirement to
have explicit authorization for non-trivial work.
Code

eval/measurements/README.md[R1-4]

+# Measurement manifests
+
+Per-agent YAML that selects which **eval measurement** scorers run after a
+managed agent job (`fullsend eval-measure`). This is **not** the functional
Relevance

● Weak

Similar “add explicit authorization/linked issue” governance suggestions were rejected previously.

PR-#476
PR-#631

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 1538390 requires an explicit linked issue for non-trivial changes. The diff adds a new
eval/measurements/ feature surface (manifests + documentation), but the provided PR description
contains no linked issue reference (e.g., Fixes #...).

eval/measurements/README.md[1-4]
Skill: pr-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR introduces a non-trivial new feature/config surface (measurement manifests) but has no linked issue authorizing the work in the PR description.

## Issue Context
Compliance requires non-trivial changes (20+ changed lines or structural changes) to include an explicit linked issue (e.g., `Fixes #123` / `Refs #123`, or a full issue URL) that authorizes the change.

## Fix Focus Areas
- eval/measurements/README.md[1-4]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 55 rules
✅ Skills: 4 invoked
  code-review
  code-implementation
  pr-review
  docs-review

Grey Divider

Tip of the day
💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread eval/measurements/code.yaml
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 11:52 AM UTC · Ended 11:54 AM UTC

Commit: fe9703a · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:55 AM UTC · Completed 12:08 PM UTC

Commit: c1dbd68 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This PR modifies .pre-commit-config.yaml, a protected governance/infrastructure file. The PR has no linked issue authorizing the change. Human approval is always required for protected-path changes.
    Remediation: Link an authorizing issue in this repository, or obtain explicit human approval for governance file modifications.

Low

  • [missing-authorization] — This PR is a non-trivial feature addition (14 files, 800+ lines) with no linked issue in fullsend-ai/agents. The companion PR feat(eval)!: add eval measurements and EM-001 trace_fitness scorer fullsend#6036 and ADR 0087 provide cross-repo architectural context, but an issue in this repo would improve traceability.
  • [edge-case] eval/lint-measurements.sh:173 — The nested-list detection if dash_indent > (list_indent or 0) and current is not None skips the nesting check for the first list item (when current is None). A first dash at a deeper indent than subsequent dashes would set the wrong list_indent baseline. Practical risk is minimal since shipped manifests use consistent indentation.
  • [code-organization] Makefile:64 — The new test entry eval/lint-measurements-test.sh is inserted between hack/ and .github/ test entries rather than near the other eval/ test (eval/scripts/scrub-eval-results-test.sh). Minor ordering inconsistency.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This PR modifies .pre-commit-config.yaml, which is a protected governance/infrastructure file. The PR has no linked issue justifying changes to protected paths. Human approval is required for protected-path changes.
    Remediation: Link an authorizing issue to this PR, or obtain explicit human approval for the protected-path change.

Low

  • [redundant validation] eval/lint-measurements.sh:305 — The version field regex in main() overlaps with parse_scalar's validation for present-but-invalid values. The main() check is not dead code — it also catches missing version keys where it.get('version', '') returns ''. The overlap is partial, not total.

  • [missing-authorization] No linked GitHub issue for a 769-line, 14-file feature PR. The PR body references companion PR fullsend#6036 and ADR 0087, providing clear context, but non-trivial changes should have explicit authorization via a linked issue per project policy.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This file is under a protected path. The PR has no linked issue in fullsend-ai/agents providing authorization for modifying governance/infrastructure files. The change adds a lint-measurements pre-commit hook entry, which is consistent with the PR's intent and mirrors the existing lint-agent-docs hook pattern, but human approval is always required for protected-path changes regardless of context.
    Remediation: Obtain explicit human approval for this infrastructure change, or link to a tracking issue in fullsend-ai/agents that authorizes the .pre-commit-config.yaml modification.

Low

  • [missing-authorization] This PR introduces a non-trivial new feature (measurement manifests, lint tooling, pre-commit hooks, documentation — ~600 lines across 14 files) with no linked issue in the fullsend-ai/agents repository. The PR body references a companion platform PR (fullsend#6036 / ADR 0087), which provides design-level context and adequate authorization signal.
    Remediation: Consider creating a tracking issue in fullsend-ai/agents for traceability.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This PR modifies .pre-commit-config.yaml, a protected governance/infrastructure file. The PR has no linked issue to establish authorization for this change. Human approval is always required for protected-path changes.
    Remediation: Link an authorizing issue to this PR that justifies modifying .pre-commit-config.yaml, or obtain human approval for the protected-path change.

Low

  • [naming-convention] eval/lint-measurements.sh:1 — The new pre-commit local hook entry script uses a .sh extension (eval/lint-measurements.sh), while the existing analogous hook script has no extension (hack/lint-agent-docs). Both are language: script hooks serving the same purpose. The codebase convention for pre-commit local-hook linter scripts is extensionless.

  • [missing-authorization] No linked issue for this non-trivial feature PR (578 additions, 14 files). The PR body provides context via companion PR feat(eval)!: add eval measurements and EM-001 trace_fitness scorer fullsend#6036 and ADR 0087 but no authorizing issue is linked in this repository for scope traceability.


Labels: PR adds new eval/measurements subsystem (YAML manifests, linter, tests) — feature addition, not purely documentation


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (4)

Review

Findings

High

  • [protected-path] .pre-commit-config.yaml — This PR modifies .pre-commit-config.yaml, which is a protected governance/infrastructure file. The PR has no linked issue to authorize changes to protected paths. Human approval is always required for protected-path changes.
    Remediation: Ensure a human reviewer explicitly approves the changes to .pre-commit-config.yaml.

Low

  • [variable-naming-consistency] eval/lint-measurements.sh:24 — Counter variable naming is inconsistent with eval/lint-cases.sh, the closest sibling linter in the eval/ tree. eval/lint-cases.sh uses uppercase ERRORS=0, while this script uses lowercase errors=0 and file_count=0. Although hack/lint-agent-docs uses lowercase, eval/lint-cases.sh is the more relevant local precedent.

  • [error-message-consistency] eval/lint-measurements.sh:179 — Final error message uses failure(s) while the sibling linter eval/lint-cases.sh uses failures without the parenthetical.

  • [yaml-document-marker-consistency] eval/measurements/code.yaml:1 — The six new measurement manifests use the YAML document start marker (---) on line 1, while all existing YAML files under eval/ (e.g. eval/code/eval.yaml, eval/triage/cases/001-bug-url-encoding/input.yaml) omit it. This introduces a minor style inconsistency within the eval/ tree.

  • [incomplete documentation] LOCAL.md:133LOCAL.md describes the eval/ directory as containing only 'functional test scenarios that run agents against ephemeral GitHub repos and score the results.' With this PR, eval/ now also contains eval/measurements/ — default online-scoring measurement manifests consumed by fullsend eval-measure. The description is not wrong but is now incomplete; eval/README.md (updated in this PR) serves as the canonical reference.
    Remediation: Add a brief mention of measurement manifests to the eval/ description in LOCAL.md.

  • [missing documentation] README.md:18 — The 'Repository structure' section lists all major directories but omits eval/, a significant top-level directory. While this omission predates this PR, it becomes more noticeable with measurement manifests added.
    Remediation: Add eval/ to the directory listing in README.md.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (5)

Review

Findings

Low

  • [yaml-document-marker-consistency] eval/measurements/code.yaml:1 — The six new measurement manifests use the YAML document start marker (---) on line 1, while all existing YAML files under eval/ (e.g. eval/code/eval.yaml, eval/triage/cases/001-bug-url-encoding/input.yaml) omit it. This introduces a minor style inconsistency within the eval/ tree. Since these files live in their own subdirectory and serve a distinct purpose, this is cosmetic rather than harmful, but dropping the --- would align with the established convention.

  • [incomplete documentation] LOCAL.md:133 — LOCAL.md describes the eval/ directory as containing only 'functional test scenarios that run agents against ephemeral GitHub repos and score the results.' With this PR, eval/ now also contains eval/measurements/ — default online-scoring measurement manifests consumed by fullsend eval-measure. The description is not wrong but is now incomplete; a reader following this guide would not learn that eval/ also houses measurement manifests.
    Remediation: Add a sentence after line 135 mentioning the measurement manifests, e.g.: 'The eval/measurements/ subdirectory holds default online-scoring manifests (see eval/measurements/README.md).'

Previous run (6)

Review

Findings

Low

  • [documentation-link-consistency] eval/README.md:151 — Reference to 'fullsend ADR 0087' uses plain text while adjacent references in the same sentence use markdown hyperlinks (e.g., [fullsend#6036](...)). The eval/measurements/README.md already hyperlinks the same ADR with a direct URL, so the link target is known and available.

Labels: PR adds YAML policy files and documentation for eval measurement manifests

Previous run (7)

Review

Findings

Low

  • [scope-architectural-inconsistency] eval/measurements/prioritize.yaml — Measurement manifest exists for the 'prioritize' agent, but no functional eval directory exists at eval/prioritize/. The measurements README explains these are separate systems, but an explicit note that measurement coverage is independent of functional eval coverage would prevent future confusion.

  • [scope-architectural-inconsistency] eval/measurements/retro.yaml — Same as above for the 'retro' agent — no functional eval directory at eval/retro/. Consider a one-line clarification in the measurements README.

  • [scope-missing-dependency] eval/measurements/README.md:14 — References 'fullsend ADR 0087' as the authoritative specification without a direct URL. The ADR lives in the external fullsend-ai/fullsend repository (a standard cross-repo reference pattern), but adding a direct link when the ADR is published would improve discoverability.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 10, 2026
@ascerra
ascerra force-pushed the feat/eval-measurements-manifests branch from c1dbd68 to 74b8959 Compare August 10, 2026 13:27
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 1:28 PM UTC · Ended 1:33 PM UTC

Commit: 74b8959 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:48 PM UTC

Commit: 93d3db9 · View workflow run →

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM: No CI/pre-commit validation for eval/measurements/*.yaml — bad scorer/agent values silently no-op forever

(Referencing .pre-commit-config.yaml line 49, which isn't part of this PR's diff, so noting it here in the review body instead of inline.)

Confirmed on head 93d3db9f: the repo has a pre-commit hook lint-agent-docs (./hack/lint-agent-docs) scoped via files: ^(harness/|docs/|hack/lint-agent-docs) that validates every harness/*.yaml has a valid doc: field, and a separate eval/lint-cases.sh that validates eval/<agent>/cases and eval.yaml judges — but neither covers the new eval/measurements/*.yaml files added in this PR, and no .github/workflows/*.yml step does either. Only the generic check-yaml pre-commit hook (syntax-only) applies.

On the consumer side, in the companion PR fullsend-ai/fullsend#6036, internal/evalmeasure/registry.go's ScoreTrace has:

switch m.Scorer {
case ScorerFitness: ...
default: // Unknown scorers are skipped (forward-compatible)
}

i.e. a typo'd scorer: value (e.g. trace-fitness with a hyphen) or a misspelled agent: value that never matches a trace's agent name would pass this repo's CI cleanly, merge, and then silently produce zero measurements for that agent in production with no error/log/CI failure.

The six current manifests all correctly use the literal trace_fitness matching const ScorerFitness = "trace_fitness" in fitness.go, so nothing is broken today, but there's no guardrail against this on the next manifest edit or new-agent addition.

Suggestion: Add a small lint script (sibling to hack/lint-agent-docs, or extend eval/lint-cases.sh) that validates eval/measurements/*.yaml: agent: matches an existing agents/<name>.md, measurements[].id is unique per file, and .scorer/.version are present/well-formed (and, once the fullsend-side registry is stable, checked against a known-scorer allow-list). Wire it into .pre-commit-config.yaml (files: ^eval/measurements/) so a typo fails CI instead of silently no-op'ing at runtime.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the documentation Improvements or additions to documentation label Aug 10, 2026
@ascerra
ascerra marked this pull request as draft August 11, 2026 01:40
@ascerra
ascerra force-pushed the feat/eval-measurements-manifests branch from 93d3db9 to 228c7b5 Compare August 13, 2026 11:49
@ascerra
ascerra marked this pull request as ready for review August 13, 2026 11:50
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Ended 11:50 AM UTC

Commit: 228c7b5 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 11:51 AM UTC · Ended 12:04 PM UTC

Commit: 228c7b5 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 228c7b5

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:51 AM UTC · Completed 12:04 PM UTC

Commit: 228c7b5 · View workflow run →

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review sweep — 3 findings (0 duplicates skipped after checking existing inline comments/reviews on this PR).

Comment thread eval/measurements/review.yaml Outdated
Comment thread eval/measurements/README.md Outdated
Comment thread eval/measurements/README.md Outdated
@ascerra
ascerra force-pushed the feat/eval-measurements-manifests branch from 228c7b5 to b1de714 Compare August 17, 2026 19:31
@ascerra

ascerra commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

@waynesun09 on the 10 Aug review-body finding (no CI for eval/measurements/*.yaml):

Fixed in b1de714. Added eval/lint-measurements.sh (wired in .pre-commit-config.yaml as lint-measurements, files ^eval/measurements/) plus eval/lint-measurements-test.sh on make script-test.

It fails CI on: unknown scorer: (allow-list is trace_fitness, matching fullsend ScorerFitness), missing/duplicate id, uppercase ids, missing/non-integer version, agent: that does not match agents/<name>.md, and filename stem ≠ agent: (jobs fetch ${AGENT}.yaml). A hyphenated trace-fitness typo now errors instead of silently scoring nothing.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:33 PM UTC · Completed 7:48 PM UTC

Commit: b1de714 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 20, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:06 AM UTC · Completed 5:21 AM UTC

Commit: 555f259 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

Comment thread eval/lint-measurements.sh
ascerra and others added 13 commits August 20, 2026 10:35
Introduce per-agent measurement manifest YAML files that select which
eval measurement scorers run after managed agent jobs. Every agent
except scribe gets trace_fitness (EM-001) at initial ship.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Lowercase EM-001 to em-001 in README to match manifest YAML files.
Add --- document start markers to all measurement manifests for
consistency with harness/*.yaml convention.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Replace ambiguous "every listed agent" phrasing with an explicit list
of the six agents that enable trace_fitness at initial ship.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Spell out agents@v0 defaults vs local overrides, engine-vs-policy split
with fullsend#6036, and when a change needs fullsend vs agents.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Match eval/measurements/README.md link style for ADR 0087 so the
measurement manifest section uses consistent markdown hyperlinks.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Keep the measurement id casing aligned with manifest id fields.
Point readers at fullsend#6036 for ADR 0087 until that PR lands on main.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Reject unknown scorers, missing fields, duplicate ids, and agent
names that do not match agents/<name>.md so a typo fails CI instead
of silently producing zero scores.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Rewrite the pre-commit linter in python3 so it runs on macOS bash 3.2,
fails closed on unsupported YAML, and documents em-001 as an agents-repo
style rule. Describe the SHA-pinned v0 GetRef fetch instead of a floating
raw URL, drop eval/ --- markers, and mention eval/ in README and LOCAL.md.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Fail closed on unknown top-level keys after the measurements list,
accept optional name: matching fullsend MeasurementSpec, drop the
false thresholds claim, and document GitLab + fetch-allowlist limits.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Match YAML comment rules so code#x / trace_fitness#typo fail closed,
reject quoted version and duplicate top-level keys that yaml.v3 rejects,
and align the GitLab GetRef docs with the public unauthenticated fetch.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Reject nested block maps under measurement fields and track per-item
duplicate keys; note empty top-level agent: so a later agent: is a
duplicate. Correct README GH_TOKEN wording for the managed Action.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Close the remaining LoadRegistry fail-open where name: {…}/[…] and
unquoted null/bool literals linted green while yaml.v3 hard-fails.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Preserve '#' inside quoted scalars so residual-hash rejection runs
instead of silently truncating; drop redundant version regex in main.

Signed-off-by: Adam Scerra <ascerra@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@ascerra
ascerra force-pushed the feat/eval-measurements-manifests branch from 555f259 to 2df9633 Compare August 20, 2026 14:40
@ascerra
ascerra requested a review from waynesun09 August 20, 2026 14:40
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 20, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:42 PM UTC · Completed 3:00 PM UTC

Commit: 2df9633 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread eval/lint-measurements.sh
Comment thread Makefile

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full change at head 2df9633. All prior findings (2 HIGH + 4 MEDIUM/LOW from earlier review rounds) are fixed as of 2a54535: README no longer documents an unpinned raw-URL fetch, lint-measurements.sh is bash-3.2-compatible with fail-closed parsing, casing/YAML-marker/error-message consistency issues resolved. The one disputed finding (review.yaml's em-001 work-item check) is resolved by ascerra's rebuttal — fullsend#5622 added a PR-env fallback to work-item resolution (merged, in v0.33.0+), demonstrated working on a live production run; my original concern was based on stale code.

Protected-path change to .pre-commit-config.yaml (adds one scoped lint-measurements hook) reviewed and approved by a human maintainer.

@ascerra
ascerra added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit 56e0a7b Aug 20, 2026
58 checks passed
@ascerra
ascerra deleted the feat/eval-measurements-manifests branch August 20, 2026 16:14
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 20, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 4:16 PM UTC · Completed 4:29 PM UTC

Commit: 2df9633 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #722 -- feat(eval): add measurement manifests for online scoring

Workflow: Human-authored PR by ascerra adding default measurement manifests (eval/measurements/), a YAML lint script, pre-commit integration, and documentation. 826 additions across 14 files, 13 commits over 10 days (Aug 10--20). No linked issue; companion engine PR: fullsend-ai/fullsend#6036.

Review cycles: The review agent ran 13 times (8 producing findings); a human reviewer (waynesun09) conducted 4 review sweeps.

Review agent behavior: Approved 3 times (Aug 10--13) with only LOW findings (documentation formatting, YAML markers, architectural nits). Switched to CHANGES_REQUESTED on Aug 17 solely due to a protected-path HIGH finding (.pre-commit-config.yaml modified without linked issue) -- a governance gate, not a correctness concern. The review agent never produced a correctness-based blocking finding across all 8 successful runs.

Human reviewer findings the review agent missed:

  • HIGH: Bash 4+ constructs (mapfile, declare -A) in a pre-commit hook breaking macOS bash 3.2 -- led to full rewrite in Python3
  • HIGH: README documented an insecure unpinned URL fetch mechanism when the companion PR's actual implementation uses authenticated SHA-pinned GetRef
  • MEDIUM: Missing validation/lint coverage for new measurement manifest files -- catalyst for the entire lint script
  • MEDIUM: Process substitution silently swallowing Python parser errors
  • MEDIUM: Lint enforcing stricter ID format than fullsend's actual LoadRegistry requires
  • MEDIUM: review.yaml enabling em-001 when work_item sub-check is structurally capped for review traces

What the review agent did well: Protected-path detection was correct and persistent. Missing-authorization finding (no linked issue for 800+ line PR) was appropriate. LOW documentation consistency findings were accurate.

Supporting evidence for existing issues:

Proposals filed: 2

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants